Skip to content

fix: honor prioRequestsProcessing default and stop sharing cached params#52

Merged
jkyberneees merged 1 commit into
masterfrom
fix/prio-default-and-shared-params
May 31, 2026
Merged

fix: honor prioRequestsProcessing default and stop sharing cached params#52
jkyberneees merged 1 commit into
masterfrom
fix/prio-default-and-shared-params

Conversation

@jkyberneees
Copy link
Copy Markdown
Collaborator

Summary

Two correctness bugs found during a code review of the router core, each validated by a regression test that fails on master and passes with the fix.

#1prioRequestsProcessing default silently disabled by any config (index.js)

The documented default: true lived only in the parameter default:

module.exports = (config = { prioRequestsProcessing: true }) => { ... }

That default only applies to the zero-arg call. Any real-world call that passes config — zero({ router }), zero({ errorHandler }), zero({ server }) — leaves config.prioRequestsProcessing as undefined (falsy), so the optimization is silently turned off, contradicting the docs/landing-page config table.

The existing prio-request-processing.test.js never caught this because all three cases pass the flag explicitly.

Fix: resolve inside the body — const prioRequestsProcessing = config.prioRequestsProcessing ?? true.

#2 — Cached params shared by reference across requests (lib/router/sequential.js)

router.lookup caches trouter's whole match object (including its params) in the LRU cache (on by default, cacheSize = -1). On a cache hit it assigned req.params = params by reference, so every request to the same method+path shared one mutable object. A non-idempotent middleware mutation bled into later requests.

Demonstrated by the new test: a handler doing req.params.id = req.params.id + '!' on route /item/:id returns x! on the first hit but x!! on the second (shared object) before the fix.

Fix: shallow-copy on assignment — req.params = params ? { ...params } : Object.create(null). The pre-existing else if (params) branch already copied into an existing req.params, so only the direct-assignment path needed the change.

Validation

  • New tests/regression-findings.test.js — both tests fail on master (expected undefined to equal true; expected 'x!!' to equal 'x!') and pass with the fix.
  • Full suite: 69 passing (was 64), coverage 97% (gate is 85%).
  • standard lint clean.

🤖 Generated with Claude Code

Two correctness fixes, each validated by a regression test that fails on
the prior code:

1. prioRequestsProcessing default — the `true` default lived only in the
   parameter default `(config = { prioRequestsProcessing: true })`, so any
   `zero({ ...otherConfig })` call left the flag `undefined` (falsy) and
   silently disabled it, contradicting the documented `default: true`.
   Now resolved inside the body via `config.prioRequestsProcessing ?? true`.

2. Shared cached params — `router.lookup` caches trouter's full `match`
   object (including `params`) in the LRU and assigned `req.params = params`
   by reference. All requests to the same method+path shared one mutable
   object, so a non-idempotent middleware mutation (e.g. `req.params.id += x`)
   bled into later requests. Now shallow-copied on assignment.

Regression test: tests/regression-findings.test.js (fails before, passes after).
Full suite: 69 passing, coverage 97%.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jkyberneees jkyberneees merged commit 11ea6e1 into master May 31, 2026
5 checks passed
jkyberneees added a commit that referenced this pull request May 31, 2026
- Bump package.json 5.0.0 -> 5.0.1 for the patch release (fixes from #52,
  perf from #53).
- Remove the TRON wallet address from the README Support section; PayPal
  link retained.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant